Skip to content

Conversation

@Jutho
Copy link
Member

@Jutho Jutho commented Apr 17, 2025

Very much work in progress.

A first draft for CUDA support, for now only for QR and untested. My local machine with GPU has currently some issues with installing CUDARuntime.jl, which is a major hinderance for further developing and testing this. I hope this gets resolved soon.

Also, most of the code in yacusolver.jl is either Copilot generated or merely copy pasted and should definitely be ignored at this point.

@Jutho
Copy link
Member Author

Jutho commented Apr 22, 2025

Ok I got QR and LQ working.

I find the algorithm argument to initialize_output an annoyance often, but I now adopted the strategy to define the generic initialize_output methods with an ::AbstractAlgorithm final argument which catches all cases. If a specific algorithm does want a modified initialize_output method, it can still do so by defining a more specific method.

Has anyone experience with setting up GPU/CUDA test infrastructure? I've created test files with are working locally, but it would of course be good to also have CI for this.

@codecov
Copy link

codecov bot commented Apr 22, 2025

Codecov Report

❌ Patch coverage is 94.08100% with 19 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
ext/MatrixAlgebraKitCUDAExt/yacusolver.jl 92.74% 9 Missing ⚠️
ext/MatrixAlgebraKitCUDAExt/implementations/svd.jl 93.84% 4 Missing ⚠️
src/interface/lq.jl 20.00% 4 Missing ⚠️
...MatrixAlgebraKitCUDAExt/MatrixAlgebraKitCUDAExt.jl 75.00% 2 Missing ⚠️
Files with missing lines Coverage Δ
ext/MatrixAlgebraKitCUDAExt/implementations/qr.jl 100.00% <100.00%> (ø)
src/MatrixAlgebraKit.jl 100.00% <ø> (ø)
src/algorithms.jl 80.48% <100.00%> (ø)
src/common/initialization.jl 100.00% <100.00%> (ø)
src/implementations/eig.jl 100.00% <100.00%> (ø)
src/implementations/eigh.jl 98.41% <100.00%> (ø)
src/implementations/lq.jl 98.62% <100.00%> (+0.42%) ⬆️
src/implementations/polar.jl 100.00% <100.00%> (ø)
src/implementations/qr.jl 97.36% <100.00%> (ø)
src/implementations/schur.jl 100.00% <100.00%> (ø)
... and 5 more

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kshyatt
Copy link
Member

kshyatt commented Aug 8, 2025

Managed to rebase this locally, will work on getting the tests fixed now

@kshyatt
Copy link
Member

kshyatt commented Aug 9, 2025

BTW I think also StridedCuMatrix should work, not just CuMatrix, so I will try to update all the CUDA wrappers to reflect this (could be quite helpful for views).

Copy link
Member

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me like this!

I went a bit back and forth between the idea of defining the algorithms in the main package vs defining them in the extension. When we designed the @algdef thing I think we did it in such a way that you don't HAVE to put it in the main package, since it's technically not defining any new structs, simply adding some constants.
The main issue is that this is still a bit annoying to then use as actual qualifiers, so I think this solution is actually better.
I was wondering however if there is a way to make it such that constructing/using these algorithms when CUDA is not loaded would give an error along the lines of "this needs using CUDA".
I'm also okay with just merging this as is though.

@kshyatt kshyatt enabled auto-merge (squash) August 12, 2025 09:02
@kshyatt kshyatt merged commit 4d091cb into main Aug 12, 2025
10 checks passed
@kshyatt kshyatt deleted the jh/cuda branch August 12, 2025 09:16
m, n = size(A)
minmn = min(m, n)
At = adjoint!(similar(A'), A)
At = adjoint!(similar(A'), A)::AbstractMatrix
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, this is where the type assert was added. Is this only JET that complains, or was there really a type instability or type inference issue? I cannot see how asserting an abstract type would help with anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants